Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ce8af05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (67)
📝 WalkthroughWalkthroughComprehensive refactoring consolidating address and temporal type handling by migrating from scattered implementations to unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR continues the ENS SDK refactor by standardizing address normalization across the stack, tightening the “interpreted name” model, and updating downstream packages/apps to use the new types/helpers.
Changes:
- Introduces
NormalizedAddress+ normalization helpers and updates GraphQL/Zod/address usage to consistently use lowercase EVM addresses. - Refactors ENS name utilities around
InterpretedName(new parent/name-hierarchy helpers, removal ofNormalizedNamebrand usage) and adds a UI helper component to enforce interpreted names. - Renames root constants (
ROOT_NODE→ENS_ROOT_NODE, addsENS_ROOT_NAME) and migrates manyUnixTimestampimports to come fromenssdk.
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx | Swap UnixTimestamp type import to enssdk. |
| packages/enssdk/src/omnigraph/graphql.ts | Map GraphQL Address scalar to NormalizedAddress. |
| packages/enssdk/src/omnigraph/generated/schema.graphql | Update scalar docs to reference enssdk + lowercase address semantics. |
| packages/enssdk/src/lib/types/evm.ts | Add NormalizedAddress type and clarify EVM scalar docs. |
| packages/enssdk/src/lib/types/ens.ts | Remove NormalizedName branded type export. |
| packages/enssdk/src/lib/parse-reverse-name.ts | Normalize reverse-derived addresses via toNormalizedAddress. |
| packages/enssdk/src/lib/normalization.ts | Drop NormalizedName type-guard; tweak error messages. |
| packages/enssdk/src/lib/names.ts | Refactor hierarchy/parent helpers to use InterpretedName + ENS_ROOT_NAME. |
| packages/enssdk/src/lib/names.test.ts | Update tests for new name helpers/constants. |
| packages/enssdk/src/lib/interpreted-names-and-labels.ts | Add nameToInterpretedName; adjust casting helpers + docs. |
| packages/enssdk/src/lib/constants.ts | Add ENS_ROOT_NAME / ENS_ROOT_NODE; type ROOT_RESOURCE. |
| packages/enssdk/src/lib/address.ts | Add isNormalizedAddress/toNormalizedAddress/asNormalizedAddress. |
| packages/enssdk/src/lib/address.test.ts | Add tests for new address normalization helpers. |
| packages/ensnode-sdk/src/tokenscope/name-token.ts | Use getParentInterpretedName and handle root-parent null. |
| packages/ensnode-sdk/src/shared/zod-schemas.ts | Rename lowercase-address schema to normalized-address schema. |
| packages/ensnode-sdk/src/shared/types.ts | Source UnixTimestamp from enssdk for BlockRef. |
| packages/ensnode-sdk/src/shared/interpretation/interpret-record-values.ts | Use InterpretedName + toNormalizedAddress for record interpretation. |
| packages/ensnode-sdk/src/shared/datetime.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/shared/cache/ttl-cache.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/shared/cache/swr-cache.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/registrars/zod-schemas.ts | Use normalized-address schema for registrar payloads. |
| packages/ensnode-sdk/src/registrars/registration-lifecycle.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/omnigraph-api/example-queries.ts | Normalize hardcoded example addresses. |
| packages/ensnode-sdk/src/indexing-status/realtime-indexing-status-projection.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/cross-chain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/response.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/request.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/filters.ts | Import UnixTimestamp from enssdk. |
| packages/ensnode-sdk/src/ensapi/api/name-tokens/response.ts | Import UnixTimestamp from enssdk. |
| packages/enskit/src/react/omnigraph/index.ts | Re-export omnigraph components. |
| packages/enskit/src/react/omnigraph/components/index.tsx | Barrel export for new component. |
| packages/enskit/src/react/omnigraph/components/ensure-interpreted-name.tsx | Add EnsureInterpretedName helper component. |
| packages/ens-referrals/src/v1/referrer-metrics.ts | Normalize referrers via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts | Normalize referrer comparisons via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.ts | Normalize event referrers via toNormalizedAddress. |
| packages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.ts | Switch to normalized-address schema + uniqueness by normalized form. |
| packages/ens-referrals/src/v1/award-models/pie-split/api/zod-schemas.ts | Switch to normalized-address schema. |
| packages/ens-referrals/src/v1/api/zod-schemas.ts | Switch to normalized-address schema for requests. |
| packages/ens-referrals/src/v1/address.ts | Remove local normalizeAddress helper. |
| packages/ens-referrals/src/referrer-metrics.ts | Normalize referrers via toNormalizedAddress. |
| packages/ens-referrals/src/api/zod-schemas.ts | Switch to normalized-address schema. |
| packages/ens-referrals/src/address.ts | Remove local normalizeAddress helper. |
| packages/datasources/src/invariants.test.ts | Adjust address-lowercase invariant comment/check. |
| examples/enskit-react-example/src/DomainView.tsx | Use EnsureInterpretedName + new parent-name helper in example UI. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/ThreeDNSToken.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Resolver.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/Registrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/subgraph/plugins/subgraph/handlers/Registry.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensindexer/src/plugins/registrars/shared/lib/registrar-events.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensindexer/src/plugins/registrars/shared/lib/registrar-action.ts | Add clarifying doc comment re: toLowerCase() keying. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv2Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ETHRegistrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/EnhancedAccessControl.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE + normalize address types. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Treat event args addresses as NormalizedAddress. |
| apps/ensindexer/src/lib/trace-transaction-helpers.ts | Normalize extracted trace addresses via toNormalizedAddress. |
| apps/ensindexer/src/lib/tokenscope/seaport-types.ts | Type Seaport event addresses as NormalizedAddress. |
| apps/ensindexer/src/lib/subgraph/subgraph-helpers.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensindexer/src/lib/heal-addr-reverse-subname-label.ts | Treat event owner address as NormalizedAddress. |
| apps/ensindexer/src/config/validations.ts | Use isNormalizedAddress for contract-config validation. |
| apps/ensapi/src/omnigraph-api/schema/scalars.ts | Parse Address scalar via normalized-address schema; update docs. |
| apps/ensapi/src/omnigraph-api/schema/permissions.integration.test.ts | Update tests to expect normalized addresses/topics casing. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Update tests to expect normalized topics/from casing. |
| apps/ensapi/src/omnigraph-api/schema/account.integration.test.ts | Normalize devnet addresses and update assertions. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Swap ROOT_NODE usage to ENS_ROOT_NODE. |
| apps/ensapi/src/omnigraph-api/builder.ts | Update Address scalar output typing to NormalizedAddress. |
| apps/ensapi/src/lib/resolution/resolve-with-universal-resolver.ts | Require InterpretedName for resolution call execution. |
| apps/ensapi/src/lib/resolution/resolve-calls-and-results.ts | Tweak error recording logic around reverts vs unexpected errors. |
| apps/ensapi/src/lib/resolution/packetToBytes.test.ts | Add test coverage for encoded-labelhash DNS encoding behavior. |
| apps/ensapi/src/lib/resolution/forward-resolution.ts | Enforce InterpretedName invariant via asInterpretedName. |
| apps/ensapi/src/lib/protocol-acceleration/find-resolver.ts | Use InterpretedName throughout resolver-finding logic. |
| apps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensapi/src/lib/handlers/params.schema.ts | Use normalized-address schema for address param parsing. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.ts | Use normalized-address schema for referrer params. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api-v1.routes.ts | Use normalized-address schema for referrer params. |
| apps/ensapi/src/handlers/api/meta/realtime-api.test.ts | Move UnixTimestamp type import to enssdk. |
| apps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.ts | Use normalized-address schema for decodedReferrer filter. |
| apps/ensapi/src/handlers/api/explore/name-tokens-api.ts | Swap root constants + parent-name helper usage. |
| apps/ensadmin/src/components/indexing-status/backfill-status.tsx | Move UnixTimestamp type import to enssdk. |
| apps/ensadmin/src/app/mock/display-identity/page.tsx | Normalize selected addresses via toNormalizedAddress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR continues the enssdk refactor: Mechanically the changes are clean and well-tested. A few improvements are worth noting:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Consumer as React Component
participant urql as urql Client
participant Cache as graphcache (omnigraphCacheExchange)
participant BigIntResolver as localBigIntResolvers
participant Network as Omnigraph API
Consumer->>urql: "useOmnigraphQuery({ query, variables })"
urql->>Cache: Check normalized cache
alt Cache miss
Cache->>Network: HTTP request
Network-->>Cache: JSON response (BigInt fields as strings e.g. "1234567890")
Cache->>Cache: "Normalize & store raw string values"
end
Cache->>BigIntResolver: resolve field (e.g. registration.start)
Note over BigIntResolver: parent["start"] = "1234567890" (string)<br/>BigInt("1234567890") → 1234567890n
BigIntResolver-->>Cache: 1234567890n (native bigint)
Cache-->>urql: Typed result (BigInt fields as bigint)
urql-->>Consumer: data.domain.registration.start: bigint
Reviews (4): Last reviewed commit: "fix: name test again" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ens-referrals/src/referrer-metrics.ts (1)
67-79:⚠️ Potential issue | 🔴 CriticalReplace lowercase validation with normalized-address validation.
buildReferrerMetrics(Line 67) normalizes withtoNormalizedAddress, which returns EIP-55 checksummed addresses (mixed-case per viem/enssdk documentation). However,validateReferrerMetrics(Line 78) enforces lowercase viavalidateLowercaseAddress, which will reject all checksummed addresses. This causes every call to throw—a blocking bug.Suggested fix
-import { validateLowercaseAddress } from "./address"; @@ export const validateReferrerMetrics = (metrics: ReferrerMetrics): void => { - validateLowercaseAddress(metrics.referrer); + if (toNormalizedAddress(metrics.referrer) !== metrics.referrer) { + throw new Error( + `ReferrerMetrics.referrer must be normalized. Got: ${metrics.referrer}.`, + ); + } validateNonNegativeInteger(metrics.totalReferrals);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ens-referrals/src/referrer-metrics.ts` around lines 67 - 79, buildReferrerMetrics normalizes referrer with toNormalizedAddress (EIP-55 checksummed), but validateReferrerMetrics still calls validateLowercaseAddress causing valid checksummed addresses to be rejected; replace the lowercase check with the matching normalized/checksummed validator — e.g., in validateReferrerMetrics remove validateLowercaseAddress(metrics.referrer) and call validateNormalizedAddress(metrics.referrer) (or the project's EIP-55/normalized address validator) so the validation matches toNormalizedAddress, keeping the existing validateNonNegativeInteger(metrics.totalReferrals) check.apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts (1)
119-126: 🧹 Nitpick | 🔵 TrivialInternal
handleTransferfunction still usesAddresstype.The internal helper
handleTransferusesto: Address(line 125), but it receivesNormalizedAddressvalues fromhandleTransferSingleandhandleTransferBatch. Consider updating this toNormalizedAddressfor consistency.♻️ Proposed fix for type consistency
async function handleTransfer( context: IndexingEngineContext, event: EventWithArgs, eventId: string, tokenId: bigint, - to: Address, + to: NormalizedAddress, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts` around lines 119 - 126, The internal handler handleTransfer currently declares the recipient parameter as to: Address but callers (handleTransferSingle and handleTransferBatch) pass NormalizedAddress; update handleTransfer's signature to use to: NormalizedAddress for type consistency, adjust any imports to include NormalizedAddress instead of Address, and remove or update any inline casts/usages inside handleTransfer that assume Address so they work with NormalizedAddress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/lib/resolution/resolve-calls-and-results.ts`:
- Around line 200-204: The catch block that currently only calls
span.recordException(error) when error instanceof Error should be changed to
record all throwables: always call span.recordException with an Error instance
by using the original Error if error instanceof Error, otherwise create a new
Error(String(error ?? 'non-error thrown')) before recording; keep the subsequent
throw error to rethrow the original value. Update the code around the catch in
resolve-calls-and-results (where span.recordException is invoked) to handle
null/undefined and non-Error objects as described.
In `@apps/ensapi/src/omnigraph-api/schema/scalars.ts`:
- Around line 39-43: The scalar type declaration for Address is misleading:
builder.scalarType("Address", ...) still describes addresses as "in all
lowercase" but the parse logic uses makeNormalizedAddressSchema which returns
EIP-55 checksummed (mixed-case) addresses; update the description to reflect
checksummed addresses or normalized EIP-55 format and ensure serialize/parse
semantics match that description (reference builder.scalarType("Address"), the
Address type, and makeNormalizedAddressSchema).
In `@apps/ensindexer/src/config/validations.ts`:
- Around line 130-133: The validation currently sets isValidAddress using both
isAddress(...) and isNormalizedAddress(...); remove the redundant isAddress call
and set isValidAddress by only calling
isNormalizedAddress(contractConfig.address as Address) (keep the variable name
isValidAddress and the contractConfig.address reference) so the check still
ensures lowercase normalized address without duplicating validation work.
In `@examples/enskit-react-example/src/DomainView.tsx`:
- Around line 85-108: Fix the typo in the inline comment inside the DomainView
component: change "redirec to /domain/etc" to "redirect to /domain/etc" (the
comment immediately above the Navigate fallback that checks params.name in the
DomainView function using useParams and Navigate).
In `@packages/datasources/src/invariants.test.ts`:
- Line 26: The assertion message in invariants.test.ts contains a grammar
mistake in the template literal for the contract address error; update the
string `The ContractConfig '${namespace}' > '${datasourceName}' >
'${contractName}' > '${contractConfig.address}' is not is lowercase format.` to
read `The ContractConfig '${namespace}' > '${datasourceName}' >
'${contractName}' > '${contractConfig.address}' is not in lowercase format.` so
the phrase uses "in lowercase format" (locate the template literal in the test
that references namespace, datasourceName, contractName, and
contractConfig.address).
In
`@packages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.ts`:
- Around line 58-60: The duplicate-check refinement currently re-normalizes
addresses by calling toNormalizedAddress(item.referrer) even though
item.referrer is already normalized by the schema; in the refinement callback
replace the toNormalizedAddress call and map directly using item.referrer (i.e.,
const addresses = items.map(item => item.referrer)) so the new Set size
comparison stays the same, and remove the unnecessary toNormalizedAddress
invocation to avoid redundant work in the refinement.
In `@packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts`:
- Around line 166-169: Change the model so disqualifications[].referrer is typed
as NormalizedAddress and ensure referrer values are normalized at the model
boundary (where disqualifications are created/loaded) rather than at every
consumer; update the disqualifications type alias and any factories/parsers to
call toNormalizedAddress once when constructing a disqualification, then in
rules.ts remove the runtime toNormalizedAddress(referrer) and the
normalizedReferrer local and compare rules.disqualifications[*].referrer
directly to referrer (now already a NormalizedAddress) so normalization is
enforced at the type level.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts`:
- Around line 130-133: The invariant
invariant_registrarActionDecodedReferrerBasedOnRawReferrer currently compares
decodedReferrer against rawReferrer using .toLowerCase(); update it to use the
same normalization as decodedReferrer by importing toNormalizedAddress from
"enssdk" and replacing the .toLowerCase() usage with
toNormalizedAddress(rawReferrer) (so both sides use
makeNormalizedAddressSchema/toNormalizedAddress), and add the import for
toNormalizedAddress at the top of the file.
In `@packages/enssdk/src/lib/constants.ts`:
- Around line 8-13: ENS_ROOT_NODE currently recreates the empty interpreted name
inline; change ENS_ROOT_NODE to use the existing ENS_ROOT_NAME constant as its
single source of truth by passing ENS_ROOT_NAME into namehashInterpretedName
(keep the Node type), i.e., replace the asInterpretedName("") usage in the
ENS_ROOT_NODE initializer with ENS_ROOT_NAME so the empty name is defined only
once and cannot drift.
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts`:
- Around line 218-224: The JSDoc for asLiteralLabel incorrectly claims it
"Validates and casts" while the implementation only performs a type cast; update
the documentation or implementation: either reintroduce runtime validation
inside the asLiteralLabel(label: Label): LiteralLabel function (perform checks
on format/characters and throw a clear error on invalid input) or change the
JSDoc to say it "Casts" (e.g., "Casts a string to a LiteralLabel without runtime
validation") so the comment accurately reflects the behavior; reference the
asLiteralLabel function to locate the change.
- Around line 29-40: nameToInterpretedName incorrectly transforms the ENS root
("") because "".split(".") yields [""] which gets encoded as a non-root label;
fix by handling the root early: in nameToInterpretedName, if name === "" return
interpretedLabelsToInterpretedName([]) (i.e., an empty labels array) before
calling .split and the label mapping logic so you don't encode a root label;
keep the rest of the mapping using asInterpretedLabel, isEncodedLabelHash,
isNormalizedLabel, encodeLabelHash, and labelhashLiteralLabel as-is for non-root
names.
---
Outside diff comments:
In `@apps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.ts`:
- Around line 119-126: The internal handler handleTransfer currently declares
the recipient parameter as to: Address but callers (handleTransferSingle and
handleTransferBatch) pass NormalizedAddress; update handleTransfer's signature
to use to: NormalizedAddress for type consistency, adjust any imports to include
NormalizedAddress instead of Address, and remove or update any inline
casts/usages inside handleTransfer that assume Address so they work with
NormalizedAddress.
In `@packages/ens-referrals/src/referrer-metrics.ts`:
- Around line 67-79: buildReferrerMetrics normalizes referrer with
toNormalizedAddress (EIP-55 checksummed), but validateReferrerMetrics still
calls validateLowercaseAddress causing valid checksummed addresses to be
rejected; replace the lowercase check with the matching normalized/checksummed
validator — e.g., in validateReferrerMetrics remove
validateLowercaseAddress(metrics.referrer) and call
validateNormalizedAddress(metrics.referrer) (or the project's EIP-55/normalized
address validator) so the validation matches toNormalizedAddress, keeping the
existing validateNonNegativeInteger(metrics.totalReferrals) check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08e38f45-8726-4f05-bc61-391d4ba45bf1
⛔ Files ignored due to path filters (1)
packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (87)
apps/ensadmin/src/app/mock/display-identity/page.tsxapps/ensadmin/src/components/indexing-status/backfill-status.tsxapps/ensapi/src/handlers/api/explore/name-tokens-api.tsapps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.tsapps/ensapi/src/handlers/api/meta/realtime-api.test.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api-v1.routes.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.routes.tsapps/ensapi/src/lib/handlers/params.schema.tsapps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.tsapps/ensapi/src/lib/protocol-acceleration/find-resolver.tsapps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensapi/src/lib/resolution/packetToBytes.test.tsapps/ensapi/src/lib/resolution/resolve-calls-and-results.tsapps/ensapi/src/lib/resolution/resolve-with-universal-resolver.tsapps/ensapi/src/omnigraph-api/builder.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/schema/account.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/permissions.integration.test.tsapps/ensapi/src/omnigraph-api/schema/scalars.tsapps/ensindexer/src/config/validations.tsapps/ensindexer/src/lib/heal-addr-reverse-subname-label.tsapps/ensindexer/src/lib/subgraph/subgraph-helpers.tsapps/ensindexer/src/lib/tokenscope/seaport-types.tsapps/ensindexer/src/lib/trace-transaction-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ETHRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/EnhancedAccessControl.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv2Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-action.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-events.tsapps/ensindexer/src/plugins/subgraph/plugins/subgraph/handlers/Registry.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Registrar.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Registry.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/Resolver.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/ThreeDNSToken.tsexamples/enskit-react-example/src/DomainView.tsxpackages/datasources/src/invariants.test.tspackages/ens-referrals/src/address.tspackages/ens-referrals/src/api/zod-schemas.tspackages/ens-referrals/src/referrer-metrics.tspackages/ens-referrals/src/v1/address.tspackages/ens-referrals/src/v1/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/pie-split/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/api/zod-schemas.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/leaderboard.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/rules.tspackages/ens-referrals/src/v1/referrer-metrics.tspackages/enskit/src/react/omnigraph/components/ensure-interpreted-name.tsxpackages/enskit/src/react/omnigraph/components/index.tsxpackages/enskit/src/react/omnigraph/index.tspackages/ensnode-sdk/src/ensapi/api/name-tokens/response.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/filters.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/request.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/response.tspackages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/cross-chain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.tspackages/ensnode-sdk/src/indexing-status/realtime-indexing-status-projection.tspackages/ensnode-sdk/src/omnigraph-api/example-queries.tspackages/ensnode-sdk/src/registrars/registration-lifecycle.tspackages/ensnode-sdk/src/registrars/zod-schemas.tspackages/ensnode-sdk/src/shared/cache/swr-cache.tspackages/ensnode-sdk/src/shared/cache/ttl-cache.tspackages/ensnode-sdk/src/shared/datetime.tspackages/ensnode-sdk/src/shared/interpretation/interpret-record-values.tspackages/ensnode-sdk/src/shared/types.tspackages/ensnode-sdk/src/shared/zod-schemas.tspackages/ensnode-sdk/src/tokenscope/name-token.tspackages/enssdk/src/lib/address.test.tspackages/enssdk/src/lib/address.tspackages/enssdk/src/lib/constants.tspackages/enssdk/src/lib/interpreted-names-and-labels.tspackages/enssdk/src/lib/names.test.tspackages/enssdk/src/lib/names.tspackages/enssdk/src/lib/normalization.tspackages/enssdk/src/lib/parse-reverse-name.tspackages/enssdk/src/lib/types/ens.tspackages/enssdk/src/lib/types/evm.tspackages/enssdk/src/omnigraph/graphql.tspackages/namehash-ui/src/components/registrar-actions/RegistrarActionCard.tsx
💤 Files with no reviewable changes (3)
- packages/ens-referrals/src/address.ts
- packages/ens-referrals/src/v1/address.ts
- packages/enssdk/src/lib/types/ens.ts
packages/ens-referrals/src/v1/award-models/rev-share-limit/rules.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/enssdk/src/lib/interpreted-names-and-labels.ts (2)
219-224:⚠️ Potential issue | 🟡 MinorMake the
asLiteralLabeldocs match the implementation.Line 220 still says this helper “validates and casts,” but Lines 223-224 now do a plain cast only. Either restore validation or update the JSDoc to avoid implying runtime checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 219 - 224, The JSDoc for asLiteralLabel incorrectly claims it "validates and casts" while the implementation only does a TypeScript cast; either implement runtime validation inside asLiteralLabel (e.g., check Label shape and throw or return a safe result) or update the JSDoc to remove "validates" and state it simply performs a TypeScript cast/typing assertion. Refer to the function name asLiteralLabel and ensure the chosen change is consistent across the file (update tests/docs if needed).
30-40:⚠️ Potential issue | 🟠 MajorHandle the ENS root before splitting labels.
Line 33 still turns
""into[""], sonameToInterpretedName("")encodes an empty label instead of preserving the root name.💡 Suggested fix
export function nameToInterpretedName(name: Name): InterpretedName { + if (name === "") return interpretedLabelsToInterpretedName([]); + return interpretedLabelsToInterpretedName( name .split(".")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts` around lines 30 - 40, The function nameToInterpretedName currently splits the ENS root ("") into [""] and then encodes that empty label; instead, detect the root case up-front and return the interpreted-root representation (i.e., call interpretedLabelsToInterpretedName with an empty array) when name === "" (or canonical root form), otherwise proceed with the existing split-and-map logic; update nameToInterpretedName to short-circuit on the empty root before performing split and referencing helpers like isEncodedLabelHash, isNormalizedLabel, encodeLabelHash, labelhashLiteralLabel, asLiteralLabel, and asInterpretedLabel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensindexer/src/config/validations.ts`:
- Line 130: Fix the grammar in the error message string in validations.ts:
change "not a enssdk#NormalizedAddress" to "not an enssdk#NormalizedAddress"
where the message is composed using config.namespace, datasourceName,
contractName and contractConfig.address so the corrected article applies to the
identifier enssdk#NormalizedAddress in that error text.
In `@examples/enskit-react-example/src/DomainView.tsx`:
- Line 58: The UI currently returns a message exposing internal type name
"InterpretedName" (in the conditional that checks data?.domain), so update the
user-facing text in DomainView (the early return that uses data?.domain and
name) to a friendly phrase like "A domain named '{name}' was not found." or "No
domain found with the name '{name}'.", replacing "InterpretedName" with plain
language.
In `@packages/ens-referrals/src/address.ts`:
- Line 6: Update the thrown error message in
packages/ens-referrals/src/address.ts to use the new NormalizedAddress
terminology: replace the existing message "Invalid address: '${address}'.
Address must be a lowercase EVM Address." with a message that references
"NormalizedAddress" (e.g., "Invalid address: '${address}'. Address must be a
NormalizedAddress.") so diagnostics match the refactor; locate the throw new
Error(...) in the address validation logic where the variable address is used
and change only the message text.
In `@packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts`:
- Around line 39-41: Create a shared alias (e.g., NonEmptyInterpretedName) that
documents the “non-empty InterpretedName” invariant and replace the inline
typing on both columns with that alias; specifically add a type alias named
NonEmptyInterpretedName documenting the non-empty contract and then change
occurrences like value: t.text().notNull().$type<InterpretedName>() (and the
other occurrence around lines 131-133) to use $type<NonEmptyInterpretedName>()
so the invariant is declared once and reused.
In `@packages/enssdk/src/lib/address.test.ts`:
- Around line 47-53: Tests currently assert a broad /does not/ message which may
match unrelated errors; update the two specs ("should throw for a partial
address" and "should throw for a non-hex string") to assert the actual error
shape/message thrown by toNormalizedAddress instead of the vague regex — either
use toThrowError(/exact expected message/) with the precise message the function
produces (e.g., /invalid address/i) or assert the concrete error type with
toThrow(TypeError) if it throws a TypeError; change the expectations around
calls to toNormalizedAddress(...) accordingly.
---
Duplicate comments:
In `@packages/enssdk/src/lib/interpreted-names-and-labels.ts`:
- Around line 219-224: The JSDoc for asLiteralLabel incorrectly claims it
"validates and casts" while the implementation only does a TypeScript cast;
either implement runtime validation inside asLiteralLabel (e.g., check Label
shape and throw or return a safe result) or update the JSDoc to remove
"validates" and state it simply performs a TypeScript cast/typing assertion.
Refer to the function name asLiteralLabel and ensure the chosen change is
consistent across the file (update tests/docs if needed).
- Around line 30-40: The function nameToInterpretedName currently splits the ENS
root ("") into [""] and then encodes that empty label; instead, detect the root
case up-front and return the interpreted-root representation (i.e., call
interpretedLabelsToInterpretedName with an empty array) when name === "" (or
canonical root form), otherwise proceed with the existing split-and-map logic;
update nameToInterpretedName to short-circuit on the empty root before
performing split and referencing helpers like isEncodedLabelHash,
isNormalizedLabel, encodeLabelHash, labelhashLiteralLabel, asLiteralLabel, and
asInterpretedLabel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f31fc64-0b5d-4403-bdb0-b4d60d9244eb
📒 Files selected for processing (11)
apps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensindexer/src/config/validations.tsexamples/enskit-react-example/src/DomainView.tsxpackages/ens-referrals/src/address.tspackages/ens-referrals/src/referrer-metrics.tspackages/ens-referrals/src/v1/address.tspackages/ens-referrals/src/v1/award-models/rev-share-limit/rules.tspackages/ens-referrals/src/v1/referrer-metrics.tspackages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.tspackages/enssdk/src/lib/address.test.tspackages/enssdk/src/lib/interpreted-names-and-labels.ts
packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts
Show resolved
Hide resolved
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Awesome updates here 😄 Reviewed and shared some suggestions 👍 Appreciate your advice!
| options: Omit<Parameters<typeof _resolveForward>[2], "registry">, | ||
| ): Promise<ForwardResolutionResult<SELECTION>> { | ||
| // Invariant: Name must be an InterpretedName | ||
| const interpretedName = asInterpretedName(name); |
There was a problem hiding this comment.
We have a related issue open: #1032
What do you suggest we do in relation to issue 1032 within the scope of this PR (PR 1908)? Should we work to confirm that issue 1032 can be successfully closed, or maybe keep issue 1032 open and make any new relevant comments on it that might help us with working on it later?
apps/ensapi/src/omnigraph-api/schema/permissions.integration.test.ts
Outdated
Show resolved
Hide resolved
| * Validates and casts a string to a {@link LiteralLabel}. | ||
| * A LiteralLabel is a label as it literally appears onchain. | ||
| */ | ||
| export function asLiteralLabel(label: Label): LiteralLabel { |
There was a problem hiding this comment.
Maybe we completely remove the Label and Name type aliases? I worry they will be a source of problems and that only our more precise type names should be used.
The only case I can think of right now that's not covered by our more formal type names would be what happens when we beautify an InterpretedName. This doesn't return an InterpretedName. Instead it returns what we might give a new type alias for: a BeautifiedName, which is the same as an InterpretedName except it loosens the constraints such that a label might be normalizable but not necessarily normalized.
In other words:
- An
InterpretedNameis all normalized labels or encoded labelhashes. - A
BeautifiedNameis all normalizable labels or encoded labelhashes.
When we display names in a UI we should render them as BeautifiedName values.
Please also note that I believe raffy's beautify name function won't properly handle labels that are encoded labelhashes and would throw on those. So we would need to implement our own wrapper around raffy's beautify name function to correctly convert any InterpretedName into a BeautifiedName.
Appreciate your advice 👍
There was a problem hiding this comment.
@shrugs I continue to think it would be nice if we completely removed all use of the generic Name or Label types across our whole monorepo.
Please see my other comments suggesting the introduce new types for UserInputName and UserInputLabel.
We can then also add types for BeautifiedName and BeautifiedLabel based on the definition I shared in the parent comment I'm now replying to.
For our utility functions that work on unknown name / label values, those could operate on raw string values as we haven't validated yet if they are an InterpretedName (etc..) or not.
| */ | ||
| export function asLiteralLabel(label: string): LiteralLabel { | ||
| if (label === "") throw new Error("LiteralLabel must not be empty"); | ||
| export function asInterpretedName(name: Name): InterpretedName { |
There was a problem hiding this comment.
This feedback is in relation to your work on nameToInterpretedName which is really awesome.
A worry I have is that devs using this package will be lazy with how they use asInterpretedName. Specifically worried that they will use it when parsing names from user input instead of using nameToInterpretedName.
Perhaps we should rename asInterpretedName to something that makes it more explicit it is just for "system inputs" and not user inputs? Appreciate your advice.
There was a problem hiding this comment.
hmm, i see the concern, but i'm loath to change the existing as* and to* pattern for these branded types. hopefully with enough example cases and public-facing usage of the literalNameToInterpretedName function this won't be an issue?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 169 out of 174 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/ensnode-sdk/src/shared/interpretation/interpret-record-values.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 169 out of 174 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Looks good nice work! Happy to merge when you're ready ✅
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 169 out of 174 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const getNameHierarchy = (name: InterpretedName): InterpretedName[] => { | ||
| if (name === ENS_ROOT_NAME) return []; | ||
|
|
||
| return interpretedNameToInterpretedLabels(name).map((_, i, labels) => | ||
| interpretedLabelsToInterpretedName(labels.slice(i)), | ||
| ); |
There was a problem hiding this comment.
getNameHierarchy(ENS_ROOT_NAME) now returns []. At least one consumer (apps/ensapi/src/lib/protocol-acceleration/find-resolver.ts → findResolverWithIndex) treats an empty hierarchy as an invariant violation and throws, which means protocol-accelerated resolution can crash for the root name ("").
Either include ENS_ROOT_NAME in the returned hierarchy (e.g. [ENS_ROOT_NAME]) or ensure downstream resolver-lookup code treats the root name as a valid special-case and returns a null/empty result instead of throwing.
| export type BuilderScalars = { | ||
| ID: { Input: string; Output: string }; | ||
| BigInt: { Input: bigint; Output: bigint }; | ||
| Address: { Input: Address; Output: NormalizedAddress }; | ||
| Hex: { Input: Hex; Output: Hex }; | ||
| ChainId: { Input: ChainId; Output: ChainId }; | ||
| CoinType: { Input: CoinType; Output: CoinType }; | ||
| Node: { Input: Node; Output: Node }; |
There was a problem hiding this comment.
BuilderScalars declares the GraphQL Address scalar input type as Address, but apps/ensapi/src/omnigraph-api/schema/scalars.ts now parses and normalizes all Address inputs via makeNormalizedAddressSchema, so resolver args will always receive a NormalizedAddress.
To keep the schema builder types accurate (and avoid accidentally treating non-normalized inputs as possible in resolver code), consider changing BuilderScalars["Address"].Input to NormalizedAddress (or to a union/type that reflects the actual post-parse shape).
closes #1876
Reviewer Focus (Read This First)
InterpretedNames — deferring further constraints to Resolution API: Further constrain the type of names used for resolution #1920NormalizedAddressis an unbranded alias for0x{string}; all indexer/ens-referrals callsites cast rather than validate — spot-check that boundary validation still happens where it matters. the important entrypoints for the ens-referrals, ensanalytics, registrar-actions, and associated modules have all been checked and are enforced by zod schemas.enskit/reactsubmodule export +EnsureInterpretedNamerelocation — verify the package surface is what you expectWhat Changed (Concrete)
NormalizedName; resolution libs + callsites now useInterpretedNameNormalizedAddress(unbranded alias for0x{string}) replacingasLowerCaseAddress+makeLowerCaseAddressSchema; updated all indexer + ens-referrals callsitesenskit/reactsubmodule export; movedEnsureInterpretedNamethereliteralNameToInterpretedNamelogic; addednameToInterpretedNamehelper and testsbigint[]support, semantic scalars (Durationmoved to enssdk)decodeEncodedReferrernow returnsNormalizedAddressparse-reverse-name;ENS_ROOT_NAME/ENS_ROOT_NODEcleanup@urql/introspectioncodegen and the separategenerated/introspection.ts, can use the gql.tada generated file for urql introspectionDesign & Planning
enssdk: Further migrate types and libs to enssdk #1876; planning was lightweight and incremental — decisions landed in commits and copilot notes rather than a design docenssdk: Further migrate types and libs to enssdk #1876Self-Review
parse-reverse-name, partial-data guard in cache-exchange, encoded labelhash validation in interpretationensure-interpreted-namestreamlined,validateAddressrefactored,toNormalizedAddressinput type relaxedNormalizedName→InterpretedNameproject-wide;Address→NormalizedAddresswhere the invariant holdsreadFragmentre-export inenskit/react/omnigraph; removedNormalizedNametype + schemaDownstream & Consumer Impact
NormalizedNameremoved), enskit gains/reactsubmodule, omnigraph scalars reshuffled_lib/README.md, AGENTS.md note on running test filesNormalizedAddressis intentionally unbranded — it's a documentation alias, not a runtime invariant. We should probably brand it in the near future?Testing Evidence
interpreted-names-and-labels,encoded-referrer,packetToBytes,resolve-with-universal-resolverintegration, omnigraph builderNormalizedAddresscast sites (unbranded → trust the caller)Scope Reductions
Risk Analysis
InterpretedNameis passed where a literal name was previously expected;NormalizedAddresscasts on data from untrusted sourcesPre-Review Checklist (Blocking)